Skip to content

core/bazel: fix race#135

Open
sywhang wants to merge 2 commits into
mainfrom
race-core-bazel
Open

core/bazel: fix race#135
sywhang wants to merge 2 commits into
mainfrom
race-core-bazel

Conversation

@sywhang

@sywhang sywhang commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

executeQueryInternal called cmd.Wait() before waiting for the goroutines streaming stdout/stderr. exec.Cmd.Wait closes the parent ends of the pipes as soon as the process exits, so on context cancellation the function could return — and read stderrBuf for the failure message — while a stream goroutine was still writing into it. That concurrent read/write is the data race in #134 (reproducible with go test -race -run TestExecuteQueryInternal_ContextTimeout). The same early close could also silently truncate stdout mid-parse and drop the tail of captured stderr.

Rather than joining the stream goroutines on the cancellation path, this restructures the shutdown sequence so the situation can't arise:

  • Streams are drained before the process is reaped. g.Wait() now runs before cmd.Wait(), so Wait's pipe close can never race with (or truncate) an in-flight read. Failure errors now always carry the complete captured stderr.
  • The inner goroutine/select in the stream helpers is gone. streamOutput is an inline io.Copy and the errgroup calls getQueryResult directly; both report the context error when the context has ended. With no goroutine outliving the call, the race is fixed structurally.
  • A watchdog guarantees the reads can't block forever. Reordering removes the liveness guarantee cmd.Wait's force-close used to provide: if a descendant of the killed bazel process inherits the pipe write ends and holds them open (go.dev/issue/23019), the reads would never see EOF. If the command context ends and the streams haven't finished within execcmd.GracePeriod + 5s — after the SIGTERM→SIGKILL sequence has had its chance to produce a natural EOF — the watchdog force-closes the parent pipe ends to unblock them.

execcmd.GracePeriod is exported so the watchdog delay is visibly derived from the kill schedule.

New tests cover both behaviors: TestExecuteQueryInternal_PipesHeldOpenAfterCancel simulates an orphaned descendant holding the pipes (hangs without the watchdog), and TestExecuteQueryInternal_DrainsStreamsBeforeWait has the mock Wait emulate exec's pipe close on return (truncates stdout/stderr under the old ordering). Both were validated by reintroducing each bug and watching the test fail.

Fixes #134.

There's a race in `streamOutput` and `streamAndParseTargets` where
the stderr buffer was being read / written concurrently. When the
context gets cancelled and bails, it's trying to read the buffer while
it's still being written to by the other goroutine.

The streaming goroutine should block until the other goroutine exits, so
that it doesn't attempt to read the byte buffer concurrently.

Fixes #134.
@sywhang sywhang requested review from a team as code owners June 30, 2026 23:05
Comment thread core/bazel/stream.go Outdated

select {
case <-ctx.Done():
<-done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this blocks on copying goroutine, in which case it does not make any sense to have a goroutine at all but rather block on a main thread.
what we need to have instead is a cancellable Copy, or at least a cancellable io.Reader.
This wrapper is better than nothing on pure blocking reads: https://github.com/uber/tango/pull/108/changes#diff-279a703791b2ff1b50139ea545c8aca00928b154a414330229e4891ec895aaf2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the main thread is already blocked before we call errgroup.Wait() because we're calling cmd.Wait().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

race in core/bazel

3 participants